Repository navigation
Make the OpenSearch max_result_window raise take effect after the first time - #1846
Conversation
…st time The background index sync raises index.max_result_window to 1.2x the indexed document count once an index exceeds 10,000 documents, but the PUT carried ?preserve_existing=true, which leaves an already-stored setting untouched. The window was therefore set exactly once in an index's life and never raised again. On flukebook.org the encounter window sat at 14,875 against 545,062 documents and annotation at 111,413 against 786,983; mantamatcher.org encounter 14,624 against 51,750. SearchApi answers 400 for from+size beyond the window, so API and token clients and the UI paginator were capped at a stale number. - putSettings: drop preserve_existing (the only other caller sets the analysis normalizer on a just-created, closed index). - targetMaxResultWindow: the sizing rule as a pure function. - ensureMaxResultWindow: grow-only; reads the stored value fresh and strictly, skips the update on any read failure so a transient error can never lower a window raised by hand, and drops the 60 s cache entry so the API gate sees the new window at once. - getAllVersions calls ensureMaxResultWindow instead of the inline PUT. Tests cover the sizing rule, the request shape (no parameters), grow-only, the three read-failure modes, the absent-setting default, no traffic for small indexes, and cache refresh. Plan reviewed by Codex (strict uncached read for the update decision; reuse of the getRestResponse test seam instead of a request factory). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ression test - ensureMaxResultWindow: state that the read and write are not atomic and that the background sync is the only automated writer. - putSettings: state that createIndex's normalizer definition is authoritative, so a template-supplied one is overwritten by design. - New test proves the update decision reads the live setting, not the 60 s cache: a primed stale value plus a larger live value must produce no PUT. Verified by mutation: pointing the decision at the cached reader fails this test. The cache-refresh test now also asserts the PUT's endpoint and absence of query parameters. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
UrbanFercec
left a comment
There was a problem hiding this comment.
Ran it merged into main and the production side looks right to me, the PUT drops the cache entry straight after. The CI failure is in the new test itself, details inline.
| } | ||
| return settingsJson("annotation", stored[0]); | ||
| }).when(os).getRestResponse(any(Request.class)); | ||
| assertEquals(14875, os.getMaxResultWindow("annotation")); // primes the 60 s cache |
There was a problem hiding this comment.
CI gets 10000 here because of test order, not the code. MAX_RESULT_WINDOW_CACHE is static with a 60 s expiry and surefire reuses one JVM, so SearchTokenScopeChildIndexTest cached annotation at 10000 about 50 s before this ran. The individual test above only passes because its entry had expired. A unique index name per test (plus the endpoint assert at 212) fixes both.
OpenSearch's max_result_window cache is static, shared by every test class in the JVM and kept for 60 s. Two tests read through it using "annotation" and "individual". When an earlier test class had searched one of those indexes within the last minute, the priming read returned that class's cached 10000 instead of the stubbed value and the test failed; that is what happened in CI. In the other direction, these tests left their made-up windows cached for whatever class ran next. Both tests now use index names no other test touches. Test-only; no production change. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1846 +/- ##
=======================================
Coverage 56.92% 56.92%
=======================================
Files 317 317
Lines 12867 12867
Branches 4184 4171 -13
=======================================
Hits 7324 7324
- Misses 5246 5252 +6
+ Partials 297 291 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
UrbanFercec
left a comment
There was a problem hiding this comment.
The new index names keep these two tests out of the shared cache, and CI is green on the full run now. Good to go from my side.
|
Merging with @UrbanFercec approval. Thank you @UrbanFercec! |
Problem
The background index sync raises
index.max_result_windowto 1.2× the indexed document count once an index exceeds 10,000 documents, sofrom + sizepaging can reach everything indexed. ButOpenSearch.putSettings()sent that PUT with?preserve_existing=true, which OpenSearch defines as "leave any setting that already exists unchanged". The window was therefore set exactly once in an index's life, the first sync pass after it crossed 10,000 documents, and never raised again.SearchApianswers HTTP 400 forfrom + sizebeyond the window, so API and token clients, and the UI paginator (which readsX-Wildbook-Max-Result-Window), were capped at a stale number.Measured live on 2026-11-04 before the hand fix:
_count)The encounter windows on both installs decode to ≈12,300 documents: an index filled by the background sync in slices of 2,500 first exceeds 10,000 at ≈12,500, and that first pass was the only one that ever stuck.
Fix
Single file plus its test.
putSettings: droppreserve_existing. The only other caller iscreateIndex(), which sets the analysis normalizer on a just-created, closed index; Wildbook's normalizer definition is authoritative, so a template-supplied one being overwritten is by design (noted in a comment).targetMaxResultWindow(count): the sizing rule as a pure function (1.2× the count, −1 at or below the default, clamped toint).ensureMaxResultWindow(index, count): grow-only. Reads the stored value fresh and strictly, never through the 60 s cache or the tolerantparseMaxResultWindowdefault, and skips the update on any read failure (transport, response shape, non-integer value). A transient OpenSearch error can therefore never lower a window, including one an operator raised by hand. After a successful PUT it drops the cache entry so the API gate sees the new window at once.getAllVersionscallsensureMaxResultWindowinstead of the inline PUT. A failed PUT still propagates and aborts the sync pass, exactly as before.Not changed: the scroll, the sync cadence and slice, the 1.2 factor, the 10,000 threshold, the API gate.
Tests
OpenSearchPaginationTest, 12 new cases, all written first and watched fail:Integer.MAX_VALUEforintandlongoverflowputSettings(Mockito spy ongetRestResponse): raises a smaller stored window with endpointencounter/_settingsand no query parameters; never lowers a larger stored window; skips when the settings read throws, when the response is malformed, and when the stored value is not an integer; treats an absent key as the default; sends no traffic at all for an index at or below 10,000Full suite run locally in the worktree:
Tests run: 1832, Failures: 0, Errors: 0, Skipped: 7, build success.Operations note
This PR stops the regression on the next index rebuild. Indexes that are already stuck need a one-time hand fix, which has been applied on flukebook.org and mantamatcher.org:
Size from
GET <index>/_count, not_cat/indices, whosedocs.countincludes nested documents (individual names, annotation embeddings) and overstates by 3–7×. Other installs whose indexes grew past 10,000 documents after creation have the same cap until the next sync pass with this fix deployed.Review
Plan and code reviewed by Codex (OpenAI). Its plan review rejected a first grow-only design that compared against the cached, defaulting reader, because a failed read would have lowered a hand-raised window; the strict uncached read and the three read-failure tests come from that round. Its code review converged with no Major findings; the three Minors (non-atomic write assumption, normalizer-template note, stale-cache regression test) are addressed in the second commit.
🤖 Generated with Claude Code